-
-
Notifications
You must be signed in to change notification settings - Fork 92
Add handler for permanent delivery failures (410 Gone, 404 Not Found)
#559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add handler for permanent delivery failures (410 Gone, 404 Not Found)
#559
Conversation
Summary of ChangesHello @dahlia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness of activity delivery by introducing a dedicated mechanism for handling permanent delivery failures. It allows applications to react to unrecoverable errors, such as remote inboxes returning '404 Not Found' or '410 Gone', by providing a customizable callback. This ensures that resources are not wasted on futile retry attempts and enables applications to maintain a cleaner state of their follower relationships. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a robust mechanism for handling permanent delivery failures, which is a valuable addition to the library. The changes are well-structured, including a new SendActivityError class for structured error information, an OutboxPermanentFailureHandler for application-level logic, and comprehensive test coverage. The documentation updates are also clear and helpful. I have one suggestion to make the permanent failure handling even more robust by defensively parsing URLs from the message queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds first-class handling for permanent outbox delivery failures (e.g., 404, 410) so applications can be notified and retries can be skipped, reducing wasted delivery attempts and enabling cleanup of unreachable followers.
Changes:
- Introduces
OutboxPermanentFailureHandlerandFederatable.setOutboxPermanentFailureHandler()pluspermanentFailureStatusCodes(default[404, 410]) to treat certain HTTP responses as non-retriable. - Adds
SendActivityErrorto carry structured delivery-failure details (inbox URL, status code, response body). - Propagates recipient
actorIdsthrough the outbox queue message so handlers can clean up the right followers; updates tests and docs accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/testing/src/mock.ts | Adds a no-op setOutboxPermanentFailureHandler() to keep the testing mock API-compatible. |
| packages/fedify/src/federation/send.ts | Throws SendActivityError on non-OK responses and defines the new error type. |
| packages/fedify/src/federation/send.test.ts | Adds assertions that failures throw SendActivityError with structured fields. |
| packages/fedify/src/federation/queue.ts | Extends OutboxMessage to optionally include actorIds. |
| packages/fedify/src/federation/mod.ts | Exports SendActivityError from the federation module entrypoint. |
| packages/fedify/src/federation/middleware.ts | Implements permanent-failure detection, handler invocation, and “no retry” behavior; enqueues actorIds. |
| packages/fedify/src/federation/middleware.test.ts | Adds coverage for permanent-failure handling, custom status codes, and retry-skipping behavior. |
| packages/fedify/src/federation/federation.ts | Adds the new handler registration method and the permanentFailureStatusCodes option to public types/docs. |
| packages/fedify/src/federation/callback.ts | Introduces the OutboxPermanentFailureHandler callback type. |
| packages/fedify/src/federation/builder.ts | Stores/clones the permanent failure handler; adds setter on the builder base class. |
| docs/manual/send.md | Documents permanent failure handling and permanentFailureStatusCodes. |
| docs/manual/federation.md | Documents the permanentFailureStatusCodes option. |
| CHANGES.md | Adds changelog entries for the new handler, option, and SendActivityError. |
The permanent failure handler is only invoked when the error is a SendActivityError, so the type should reflect that instead of the generic Error. This lets consumers access statusCode and responseBody without unsafe casts, and aligns the type with the documentation. fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The docs and JSDoc said handler exceptions are "silently" caught and ignored, but the implementation actually logs them with logger.error(). Updated the wording to say "caught, logged, and ignored" to accurately reflect the behavior. fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Update "handler exception is silently ignored" to "handler exception is caught and logged" to be consistent with the updated wording elsewhere. See also: fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a robust mechanism for handling permanent delivery failures, which is a valuable addition to prevent pointless retries and allow applications to clean up unreachable followers. The changes are well-structured, including a new OutboxPermanentFailureHandler, a custom SendActivityError for structured error information, and configuration options for status codes. The addition of actorIds to OutboxMessage is a thoughtful touch for handling shared inboxes. The implementation is backed by a comprehensive set of new tests, which is great to see. I have one suggestion to improve the maintainability of the new test suite.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
The onOutboxError callback is typed as returning void | Promise<void>, but was not awaited, unlike the new outboxPermanentFailureHandler. This could cause unhandled promise rejections if the callback is async. Also guard context creation with a null check on the handler to avoid unnecessary work when no handler is registered. See also: fedify-dev#559 (comment) fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Add setup() and createOutboxMessage() helpers for the permanent failure test suite to eliminate repeated KV store, queue, and federation initialization across all test steps. See also: fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The permanent failure handler is only invoked when the error is a SendActivityError, so the type should reflect that instead of the generic Error. This lets consumers access statusCode and responseBody without unsafe casts, and aligns the type with the documentation. fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The docs and JSDoc said handler exceptions are "silently" caught and ignored, but the implementation actually logs them with logger.error(). Updated the wording to say "caught, logged, and ignored" to accurately reflect the behavior. fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Update "handler exception is silently ignored" to "handler exception is caught and logged" to be consistent with the updated wording elsewhere. See also: fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The onOutboxError callback is typed as returning void | Promise<void>, but was not awaited, unlike the new outboxPermanentFailureHandler. This could cause unhandled promise rejections if the callback is async. Also guard context creation with a null check on the handler to avoid unnecessary work when no handler is registered. See also: fedify-dev#559 (comment) fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
6357309 to
39b87b2
Compare
Add setup() and createOutboxMessage() helpers for the permanent failure test suite to eliminate repeated KV store, queue, and federation initialization across all test steps. See also: fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
When an activity delivery permanently fails (e.g., the remote inbox
returns 410 Gone or 404 Not Found), retrying is pointless. This
change adds a mechanism for applications to be notified of such
failures so they can clean up unreachable followers.
Key additions:
- OutboxPermanentFailureHandler callback type, registered via
Federatable.setOutboxPermanentFailureHandler(), which receives
the failed inbox URL, activity, status code, and actor IDs.
- SendActivityError class (exported from @fedify/fedify/federation)
that captures HTTP status code, inbox URL, and response body
when delivery fails.
- permanentFailureStatusCodes option in FederationOptions to
customize which HTTP status codes are treated as permanent
failures (defaults to [404, 410]).
- actorIds field on OutboxMessage to propagate recipient actor
identifiers through the queue system.
- Automatic retry suppression for permanent failures, even when
no handler is registered.
Closes fedify-dev#548
Co-Authored-By: Claude <noreply@anthropic.com>
The permanent failure handler is only invoked when the error is a SendActivityError, so the type should reflect that instead of the generic Error. This lets consumers access statusCode and responseBody without unsafe casts, and aligns the type with the documentation. fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The docs and JSDoc said handler exceptions are "silently" caught and ignored, but the implementation actually logs them with logger.error(). Updated the wording to say "caught, logged, and ignored" to accurately reflect the behavior. fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Update "handler exception is silently ignored" to "handler exception is caught and logged" to be consistent with the updated wording elsewhere. See also: fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The onOutboxError callback is typed as returning void | Promise<void>, but was not awaited, unlike the new outboxPermanentFailureHandler. This could cause unhandled promise rejections if the callback is async. Also guard context creation with a null check on the handler to avoid unnecessary work when no handler is registered. See also: fedify-dev#559 (comment) fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Add setup() and createOutboxMessage() helpers for the permanent failure test suite to eliminate repeated KV store, queue, and federation initialization across all test steps. See also: fedify-dev#559 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
39b87b2 to
b50404e
Compare
|
Pre-release has been published for this pull request: Packages
DocumentationThe docs for this pull request have been published: |
Summary
Closes #548
When an activity delivery permanently fails (e.g., the remote inbox returns
410 Goneor404 Not Found), retrying is pointless. This PR adds a callback mechanism so applications can be notified of such failures and clean up unreachable followers.Changes
OutboxPermanentFailureHandlercallback type, registered viaFederatable.setOutboxPermanentFailureHandler(), which receives the failed inbox URL, activity, HTTP status code, error, and actor IDs.SendActivityErrorclass (exported from@fedify/fedify/federation) that captures the HTTP status code, inbox URL, and response body when delivery fails.permanentFailureStatusCodesoption toFederationOptionsto customize which HTTP status codes are treated as permanent failures (defaults to[404, 410]).actorIdsfield toOutboxMessageto propagate recipient actor identifiers through the queue system.setOutboxPermanentFailureHandler()no-op toMockFederationin@fedify/testing.Testing
SendActivityErrorin packages/fedify/src/federation/send.test.ts